Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: currency data type #4741

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: currency data type #4741

wants to merge 4 commits into from

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented Sep 27, 2024 •
edited
Loading

Explanation

There's an inconsistency regarding the currency rate data type, this PR changes it from string to number

References

Changelog

@metamask/assets-controller

  • BREAKING: RatesController data type update.
    • Change data type to number of conversionRate and usdConversionRate from Rate

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@gantunesr gantunesr requested a review from a team as a code owner September 27, 2024 14:53
ccharly
ccharly previously approved these changes Sep 27, 2024
View reviewed changes
@gantunesr @ccharly
chore: update comments
bc0de22 
Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
legobeat
legobeat suggested changes Sep 29, 2024
View reviewed changes
Copy link
Contributor

@legobeat legobeat left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like string is a good initial choice here to preserve precision and accuracy which gets lost when converting the value to floating point number?

Is there some reason why continuing to return strings is problematic here? If so, could we consider a precise decimal type (e.g. BigNumber)?

Nice properties of decimal strings is that they are (relatively) non-ambiguous, serialize cleanly to/from JSON, and don't rely on any specific implementations.

I see that CurrencyRateState is still handling and exposing rates as numbers - should we consider doing the inverse and changing that instead?

@gantunesr
Copy link
Member Author

I agree with your statement @legobeat. This was the reason I did it as string in the first place and later I decided to make this change because we only use this value for UI/UX purposes and not for any calculation. I'll close this PR if nobody opposes to it @ccharly @GuillaumeRx

@GuillaumeRx
Copy link
Contributor

I don't mind returning a string, I think this is a good technical choice. The only problem here is that the API returns a number and it is never stringified. We should stringify the return value if we want to keep the string type @gantunesr @legobeat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legobeat legobeat legobeat requested changes

@ccharly ccharly Awaiting requested review from ccharly

At least 1 approving review is required to merge this pull request.

Assignees

@gantunesr gantunesr

Labels
team-accounts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gantunesr @GuillaumeRx @ccharly @legobeat